-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix(react-start): Apply Link active properties correctly on SSR when including hash #6421
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughAdds a toggle-controlled conditional display of the current URL hash in React, Solid, and Vue starter routes, updates e2e tests to use two hash links and toggle before asserting the hash, and adjusts link active-evaluation logic in react-router and solid-router regarding hash handling. Changes
Sequence Diagram(s)(Skipped — changes are UI toggles and focused link logic updates that do not introduce a new multi-component sequential flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
|
View your CI Pipeline Execution ↗ for commit 888749e
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/react-router/src/link.tsx`:
- Around line 100-105: currentHash from useRouterState is subscribed but not
used, so hash-only navigations don't trigger recomputation of href; update the
memo dependencies (where buildLocation/_options/next/href are computed) to
include currentHash (or include currentHash in next's deps) so buildLocation is
re-run on hash changes and remove the unused-variable issue—locate the code that
computes next/_options/href (the memoized block that calls buildLocation) and
add currentHash to its dependency array.
|
Thank you for working on this! Edit: I tried to test the PR using pkg.pr.new with: "@tanstack/react-router": "https://pkg.pr.new/TanStack/router/@tanstack/react-router@6421"
"@tanstack/react-start": "https://pkg.pr.new/TanStack/router/@tanstack/react-start@6421"However, it doesn't seem to resolve the hydratation issue. And now all my links that point to the same page are considered "active", even with |
picked this up during testing and hence the draft status, digging through it to find a more holistic solution |
|
@theoludwig could you test the latest builds and advise if the problem is resolved for you. When including hashes for active options you can remove exact:true as that overrides the hash checks. |
Oh indeed, the issue is now resolved! I needed to do I end up doing something like this for checking if it needs const SidebarItemCreate = createLink(SidebarItemRaw)
export const SidebarItem: LinkComponent<React.FC<SidebarItemRawProps>> = (props) => {
return (
<SidebarItemCreate
activeProps={{
isActive: true,
}}
activeOptions={{
exact: props.hash == null,
includeHash: props.hash != null,
}}
{...props}
/>
)
}Thank you again! Looking forward to getting this released. |
This PR applies the same fix that was done in #6389 for Solid to React. As mentioned by @theoludwig in the comments this affects React as well and was missed by the tests.
This PR also updates and aligns the tests across React, Solid and Vue.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.